Expand SAM closures to anonymous classes if needed #509
Conversation
|
LTGM otherwise. |
|
Cool! Nice approach. |
|
|
||
| trait V extends Exception { def foo(x: Int): Int } | ||
|
|
||
| val v: V = (x: Int) => 2 // needs to be an anonymous class because the trait extends a non-object class |
There was a problem hiding this comment.
Where does this restriction comes from? Currently Dotty uses LambdaMetaFactory for this case and it works fine at runtime, also Exception is a subclass of Object so I don't understand what a "non-object class" is.
There was a problem hiding this comment.
"non-object class": class other than java.lang.Object.
The problem is in the runtime types. If we write
val lambda = (x => e): C
we expect that lambda.isInstanveOf[C].
There was a problem hiding this comment.
it works fine at runtime
It does not. LambdaMetaFactory explicitly checks that invokedType returns an interface.
This is both spec'd and implemented.
There was a problem hiding this comment.
object O {
abstract class D {
def foo(a: Int): Int
}
def main(args: Array[String]): Unit = {
val a: D = x => x
a.foo(1)
}
}
Exception in thread "main" java.lang.BootstrapMethodError: call site initialization exception
at java.lang.invoke.CallSite.makeSite(CallSite.java:328)
at java.lang.invoke.MethodHandleNatives.linkCallSite(MethodHandleNatives.java:296)
at O$.main(CLM.scala:5)
at O.main(CLM.scala)
Caused by: java.lang.invoke.LambdaConversionException: Functional interface O$$D is not an interface
at java.lang.invoke.AbstractValidatingLambdaMetafactory.<init>(AbstractValidatingLambdaMetafactory.java:145)
at java.lang.invoke.InnerClassLambdaMetafactory.<init>(InnerClassLambdaMetafactory.java:155)
at java.lang.invoke.LambdaMetafactory.metafactory(LambdaMetafactory.java:299)
at java.lang.invoke.CallSite.makeSite(CallSite.java:289)
... 3 more
There was a problem hiding this comment.
we expect that lambda.isInstanceOf[C].
This is the case in this example:
object Foo {
trait V extends Exception { def foo(x: Int): Int }
val v: V = (x: Int) => 2
def main(args: Array[String]): Unit = {
println(v.isInstanceOf[V])
}
}In master right now it will print "true". This makes sense: for each trait, Dotty will generate an interface, and LambdaMetaFactory will return an instance of that interface.
@DarkDimius : Your example is something else entirely: abstract class should not be SAM types, only traits can be SAM types.
There was a problem hiding this comment.
@smarter, yes, this is something else entirely, and this is what is tested here.
Quoting Martin:
"non-object class": class other than java.lang.Object.
In this example, trait V extends class Exception. So it could not be implemented by a JVM SAM.
There was a problem hiding this comment.
If you want your test to fail, try to test if v.isInstanceOf[Exception].
It's common that one wants to create class symbols with arbitary parent types, not just TypeRefs. But for the casual user it's non-obvious how to do it. Hence the new creation method.
The handling of the first parent of ClassDef was broken if that parent had type parameters. This was exposed by following commites which use ClassDef more intensively than before in creating anonymous classes representing closures.
As the name implies, this creates an anonymous class.
…winfDeep See comment in the code.
The phase replaces SAM closures with anonymous classes when necessary.
Like all other variables, pattern-bound vars need a fully defined type. I was thinking originally that demanding a fully defined selector type is sufficient to ensure this, but that's not true: An outer pattern might call a polymorphic unapply and its type variables need not be fully instantiated. With the fix, the minimized test case from ExpandSAMs works.
Removed the workaround of the original crasher which was addressed in the last commit.
Now takes a list of parent types. We needed only one parent for SAM implementation but it makes sense to generalize this. Also, removed redundant code accidentally left in.
Thanks for pointing it out, @smarter.
superClass was a duplicate; we already have one in SymDenotation, so we delete the one in SymUtils. superInterfaces is too easy to confused with the JVM notion, which is different. I replaced with directlyInheritedTraits.
Also included are - Closures implementing classes that inherit from a class other than Object - Closures that implement traits which run initialization code.
The test included here fails in backend.
|
Rebased to current master. |
|
LGTM. |
Expand SAM closures to anonymous classes if needed
Review by @DarkDimius (in particular 66e9ea8), @adriaanm